mmaprototype: fix doneFunc #158109
mmaprototype: fix doneFunc #158109
Conversation
Previously, we were incorrectly including both voterIndex and nonVoterIndex in satisfiedByReplica for voter constraints's doneFunc, which didn’t seem right. I was initially surprised that nonVoterIndex showed up at all for satisfiedByReplica of voter constraints. It might be accounting for cases where a promotion or demotion might be needed. So satisfiedByReplica[nonVoterIndex] for a voter constraint may legitimately be non-zero. TODO: Putting this up for visibility and discussion — I still need to understand the code here better to see whether this fix is right; something feels off. More tests are needed as well.
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
sumeerbhola
left a comment
sumeerbhola
left a comment
There was a problem hiding this comment.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @tbg)
pkg/kv/kvserver/allocator/mmaprototype/constraint.go line 885 at r1 (raw file):
if isVoterConstraints { // For voter constraints, only voters can satisfy them. return len(ac.satisfiedByReplica[voterIndex][j]) >= int(ac.constraints[j].numReplicas)
I think this is a step in the right direction.
We do want to not be done with a voter constraint if we are still iterating over voters (kind==voterIndex), until it is satisfied purely by voters.
But if we now on kind=nonVoterIndex we don't necessarily want to stuff them all into a single constraint that is not fully satisfied by voters. That is, we should move on once it is satisfied by voters + non-voters, giving us the change to distribute the non-voters over other constraints. This will allow for distributing the non-voters across more constraints that need satisfying.
btw, we should add test cases to TestRangeAnalyzedConstraints.
3 participants
TODO:
rac.buf.replicaConstraintIndicescockroach/pkg/kv/kvserver/allocator/mmaprototype/constraint.go
Lines 936 to 943 in 5031016
Previously, we were incorrectly including both voterIndex and nonVoterIndex in satisfiedByReplica for voter constraints's doneFunc, which didn’t seem right. I was initially surprised that nonVoterIndex showed up at all for satisfiedByReplica of voter constraints. It might be accounting for cases where a promotion or demotion might be needed. So satisfiedByReplica[nonVoterIndex] for a voter constraint may legitimately be non-zero.